emrg: GitHub device-flow auth in GUI settings (GCM rant Stage 2b) - #549
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (review angle: device-flow lifecycle + race handling)
Verified at head 0fca3fd (CI 31176833198 green). Reviewed as the pure delta vs #548 (the PR base) — 431-line device-flow increment:
- Lifecycle completeness: start → parse code+URL → background wait → timeout-kill (300s) → cancel. Every path that leaves a gh process running is accounted for.
- Cancel race handled correctly (the interesting part):
task.cancel()on a task that hasn't started its body never runs the waiter — so the proc is ALSO stored on self and_github_connect_web_cancelkills it directly. The testtest_github_connect_web_cancel_kills_pendingassertsproc.killed is Trueafter an immediate cancel (before the background task runs) — I verified this fails without the fix, so it has real discriminating power. - Probe-verified assumption:
gh auth login --webemitting the code + device URL with stdin closed was empirically confirmed in an isolated GH_CONFIG_DIR before implementation — the parse regexes match the real output format (! First copy your one-time code: ABCD-1234+https://github.com/login/device). - Negative states: gh-missing degrades, no-code output kills the proc, already-authenticated short-circuits without spawning, subprocess spawn failure degrades cleanly.
- GUI: device flow preferred when no PAT entered (PAT remains fallback per the rant), dialog with code + Open-browser (shell.openExternal) + 3s github_status polling, cancel stops polling. i18n parity 20 zh == 20 en (zero drift). RESPONSE_TYPES test extended.
- Full suite on branch: 548 py + 91 gui green.
Non-blocking observation: if the user dismisses the device dialog via Esc/overlay click (not the Cancel button), the polling interval keeps running until success/timeout — a close event listener calling stopDevicePolling() would be a nice hardening, worth a follow-up.
Preferred auth path from the Windows GCM rant (2026-08-07T10:17:27): device flow — no terminal, no GCM, no PAT needed. Daemon (emrg/server/daemon.py): - github_connect_web command: spawns `gh auth login --web` (stdin closed, prompt-free env), parses the one-time code + device URL from its output, returns them to the GUI, and keeps the process alive in a background task until the host authorizes in the browser (300s timeout-kill). - github_connect_web_cancel kills any pending flow; the proc is stored on self so a cancel racing the task start still kills the process (edge found in review: task.cancel() on a never-started task never runs the waiter). GUI: - Settings Connect button now prefers device flow when no PAT is entered (PAT remains the fallback for restricted environments). - New device-flow dialog: one-time code display, Open-browser button (shell.openExternal), 3s github_status polling until authorized. - daemon_client RESPONSE_TYPES + main IPC (emrg:githubConnectWeb / emrg:openExternal) + preload exposure; zh/en i18n. Tests: +6 Python (code/URL parsing, already-authenticated short-circuit, gh-missing degrade, no-code kill, dispatch frame, cancel-kills-pending); GUI RESPONSE_TYPES extended. Docs synced 542 -> 548 (doc-count guard).
0fca3fd to
c318ea6
Compare
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (review angle: regression surface + edge semantics)
Second review at head c318ea6 — the head was rebased onto master (after #548 merged) but the tree is byte-identical to the previously reviewed 0fca3fd (verified: 0-line diff), so no content change; the diff vs master is now exactly the 431-line device-flow delta.
- Post-rebase regression surface: the delta vs master is now precise — daemon (github_connect_web + cancel + 2 init fields), GUI (device dialog + RESPONSE_TYPES + 2 IPC + preload), tests, doc count. Nothing else. The 300s background task and proc-ref cleanup are confined to the new device-flow paths; no pre-existing daemon flow touched.
- Concurrent start semantics: a second github_connect_web cancels the first (cancel → kill proc + clear refs) before spawning — only one pending flow can exist. Verified in code path: _github_connect_web_start → _github_connect_web_cancel → spawn.
- Daemon-restart orphan: if the daemon dies mid-flow, the in-memory task is lost but the gh process is a child that becomes orphaned — gh's own device-flow expiry (~15 min) bounds it, and the next start cancels/replaces. Acceptable; the 300s task timeout is a backstop for the normal case.
- finally-cleanup correctness: _gh_web_auth_wait's finally clears both _pending_web_auth and _pending_web_auth_proc (guarded by identity check) — no dangling refs on the success path either (gh exits 0 after auth → communicate returns → finally runs).
- Mergeability: MERGEABLE + clean on the rebased head; 548 py + 91 gui verified locally on the exact head.
- GUI edge: polling stops on success AND on Cancel-button click; the non-blocking note from the first review (Esc/overlay close leaves polling until 300s timeout) remains — acceptable degradation.
2/3 — one more angle needed.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (review angle: device-flow timing/polling semantics + daemon lifecycle)
Third review at head c318ea6 (unchanged since the 2/3 reviews; CI 31177768987 green; 548 py + 91 gui verified locally).
- Polling vs process-lifetime alignment: the GUI polls github_status every 3s; the daemon's gh process waits up to 300s. GitHub's own device-flow expiry (~15 min) is the outer bound. The polling interval (3s) << timeout (300s) << device expiry (~900s) — no window where the GUI keeps polling a dead flow for longer than one poll cycle after daemon-side cleanup.
- Completion detection is race-free: gh writes its config THEN exits 0; _gh_web_auth_wait's communicate() returns only after gh exits, so a github_status poll racing the exit can only see the state AFTER gh committed its token — never a half-written config. (gh auth status would fail on a partial config, which degrades to 'not authenticated' → the GUI keeps polling → next poll sees success.)
- Multiple clients: if two GUI windows both start device flows, the second cancels the first (single _pending_web_auth slot) — only one code is ever live. The cancelled window's dialog keeps polling but the daemon's github_status now reflects the second flow's outcome; on success both windows' settings sections update. Acceptable multi-client behavior.
- Timeout-kill is the graceful exit: on 300s timeout, proc.kill() terminates gh, communicate() returns, finally clears both refs — no zombie task, no leaked process. Verified in code path + covered by test_github_connect_web_cancel_kills_pending (kill on cancel; the timeout path shares the same kill+cleanup logic).
- Mergeability: head unchanged across all 3 reviews, MERGEABLE, no conflicts with master 552d094 (rebased after #548 merged — this branch's diff vs master is exactly the device-flow delta).
Merge gate: 3 consecutive ✅ from distinct cycles (195936 lifecycle/race, 201547 regression/edges, this cycle timing/daemon-lifecycle), no ❌ in between → merging.
…p tolerance (#552) Version bump 0.2.10 → 0.2.11 across all 6 version sources (pyproject.toml / emrg/__init__.py / gui/package.json / uv.lock / make-installer.sh / build-runtime.sh). Release for Windows verification: - #541 LLM gzip body tolerance - #543 GUI message display fixes (#544 quick-ref) - #545 Windows GCM silent-fail Stage 1 - #546 Windows TUI CJK input + legacy arrow keys + /rant visibility - #548/#549/#550 GitHub auth in GUI (PAT + device flow + banner, Stage 2) - #551 quick-ref All 548 tests green. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Summary
Stage 2b of the Windows GCM popup rant (2026-08-07T10:17:27): the preferred device-flow auth path — no terminal, no GCM, no PAT needed. Stacked on #548 (PAT fallback + setup-git); when #548 merges first this PR shrinks to the device-flow delta.
Daemon (emrg/server/daemon.py)
github_connect_webcommand: spawnsgh auth login --web(stdin closed,no_prompt_env()), parses the one-time code + device URL from gh's output (probe-verified to work non-interactively), returns them to the GUI, and keeps the gh process alive in a background task until the host authorizes in the browser (300s timeout-kill).github_connect_web_cancel: kills any pending flow. The proc is stored on self so a cancel racing the task start still kills the process — review-edge:task.cancel()on a never-started task never runs the waiter body, which would otherwise leak the gh process.ok=True + userso the GUI just reflects the connected state.GUI
shell.openExternal), and 3sgithub_statuspolling until the host authorizes — then the settings section updates to connected.daemon_client.jsRESPONSE_TYPES,main.jsIPC (emrg:githubConnectWeb,emrg:openExternal), preload exposure, zh/en i18n.Tests
github_connect_web; 91 pass.Remaining (later cycles)